-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Move diagnostic generation from LibraryImportGenerator to LibraryImportDiagnosticsAnalyzer #123780
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… diagnostics Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
…itional analyzer loading Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
…till reports diagnostics) Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
jkoritzinsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot address this review.
...ime.InteropServices/gen/LibraryImportGenerator/Analyzers/LibraryImportDiagnosticsAnalyzer.cs
Show resolved
Hide resolved
...ime.InteropServices/gen/LibraryImportGenerator/Analyzers/LibraryImportDiagnosticsAnalyzer.cs
Outdated
Show resolved
Hide resolved
...raries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/DiagnosticOr.cs
Outdated
Show resolved
Hide resolved
...aries/System.Runtime.InteropServices/tests/Common/Verifiers/CSharpSourceGeneratorVerifier.cs
Show resolved
Hide resolved
…per review feedback Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
...ibraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs
Outdated
Show resolved
Hide resolved
...braries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Diagnostics.cs
Show resolved
Hide resolved
…null, remove helper method Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
…analyzer only Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
jkoritzinsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address the feedback in this review.
...ime.InteropServices/gen/LibraryImportGenerator/Analyzers/LibraryImportDiagnosticsAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.InteropServices/tests/Common/Verifiers/CSharpAnalyzerVerifier.cs
Outdated
Show resolved
Hide resolved
...aries/System.Runtime.InteropServices/tests/Common/Verifiers/CSharpSourceGeneratorVerifier.cs
Outdated
Show resolved
Hide resolved
...ibraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs
Outdated
Show resolved
Hide resolved
...ime.InteropServices/gen/LibraryImportGenerator/Analyzers/LibraryImportDiagnosticsAnalyzer.cs
Outdated
Show resolved
Hide resolved
…ion, simplify verifier, move options Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
|
@copilot Please adjust CompilationExtensions.GetEnvironmentFlags to check for DisableRuntimeMarshallingAttribute on the source assembly instead of the module. Also, add handling for the rest of the diagnostic edge cases for the remaining 5% of tests. Then, please remove the Diagnostics field from LibraryImportGenerator.IncrementalStubGenerationContext and remove downstream usages. |
…y, remove Diagnostics from generator Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
…ortDiagnosticsAnalyzer Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Addressed in commits f33ac74 and 9f85c66:
|
|
@copilot all of the test failures look like they're due to diagnostics reported by a marshalling generator resolver not being reported from the analyzers. |
…c verification Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
…nalyzer verifier Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Identified and partially fixed. The issue was that
Test status: 688/703 pass (14 failing). The 13 array marshalling tests are still failing because the analyzer returns 0 diagnostics for "Marshalling an array from unmanaged to managed requires..." cases. The |
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Description
Moves diagnostic generation/emit from the
LibraryImportGeneratorsource generator to a newLibraryImportDiagnosticsAnalyzerin the same assembly, per Roslyn team recommendation.Changes Made
1. Created
LibraryImportDiagnosticsAnalyzerSymbolEqualityComparerto compare attribute types instead of string matchingLibraryImportGeneratorOptionsonce per compilation inRegisterCompilationStartActionGetDiagnosticIfInvalidMethodForGenerationisinternal staticso the generator can call it directly2. Updated
LibraryImportGeneratorLibraryImportDiagnosticsAnalyzer.GetDiagnosticIfInvalidMethodForGenerationdirectly to filter methodsDiagnosticOrin the pipelineDiagnosticsfield fromIncrementalStubGenerationContext3. Updated
CompilationExtensions.GetEnvironmentFlagsDisableRuntimeMarshallingAttributeon the source assembly instead of the module4. Updated Test Infrastructure
TAnalyzertype parameter toCSharpSourceGeneratorVerifierDisabledDiagnostics.Add(GeneratorDiagnostics.Ids.NotRecommendedGeneratedComInterfaceUsage)toCSharpAnalyzerVerifierLibraryImportGeneratoruseLibraryImportDiagnosticsAnalyzerEmptyDiagnosticAnalyzer5. Updated Test Files
Diagnostics.cs: UsesCSharpAnalyzerVerifier(analyzer-only testing)CompileFails.cs: UsesCSharpAnalyzerVerifierfor diagnostic testsByValueContentsMarshalling.cs: Uses analyzer verifier with SYSLIB1092 re-enabled for specific testsTest Results
Key Architectural Decisions
GetDiagnosticIfInvalidMethodForGenerationmethod to share validation logic✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.